This seems a bit long and it's not completely accurate either (it accepts classes with __toString() even if they don't implement Stringable). The other alternative I considered was just string, but that might be a bit confusing to users. Is there anything better?

Let's try __stringandstringable. I can see you're already using new StringAlwaysAcceptingObjectWithToStringType in a related place.

@schlndh @ondrejmirtes After enabling this rule I noticed a lot of errors related to string|null types.

Personally, I think replacing those sprintf call arguments with $value ?? '' feels like making a computer happy. As it does exactly the same as what sprintf does for %s.

See https://3v4l.org/TlHU9#v8.4.13

I know this is a strict rule, but wonder if this was really intentional or something that should be reconsidered?

I feel like it's useful that PHPStan lets you know you're potentially passing null to sprintf. You might want to deal with null differently than what sprintf does.

I intentionally implemented it like that to match my understanding of "strict". However, I encountered the same thing: hundreds of issues like this, most of which I don't care about (though I'm fine with letting them all fall into the baseline). So I would be open to changing it. I'm just not sure what's the best way to go about it.

Strict-rules do allow $null . $string and "{$null}", so I guess this wouldn't have to be as strict either (unless it's allowed by omission rather than intentionally). At the very least it's a bit inconsistent.

On the other hand, there are third-party rules (e.g. shipmonk rules) which do prohibit it, so clearly there is also interest in the fully strict version.

Another thing to consider is whether it should be allowed only in %s or also other placehoders?

Strict-rules do allow $null . $string and "{$null}", so I guess this wouldn't have to be as strict either (unless it's allowed by omission rather than intentionally). At the very least it's a bit inconsistent.

This is an excellent point.

Another thing to consider is whether it should be allowed only in %s or also other placehoders?

I would say only for %s.

An idea: string|null should be reported only on level 8+, basicallh RuleLevelHelper should be utilized.

Unfortunately, that won't help me (level 9 🙈)

I would say only for %s.

To expand on that point: %d + null make 0, which is confusable with, well, passing an actual 0 integer. 😄 So for that reason I wouldn't allow it either.

@ondrejmirtes Re: level 8

8. report calling methods and accessing properties on nullable types

IMO that's stretching it a little bit. And it wouldn't be consistent with how string + null is handled in other cases (concatenation, interpolation, ...).

Let's consider consistency with phpstan-strict-rules (playground):

• Strict-rules prohibit bool and null in numerical operations (consistent with %d and %f).
• Strict-rules allow numeric-string in numerical operations (inconsistent with %f, %d is probably OK since float is excluded as well).
• Strict-rules prohibit general string in numerical operations (consistent with %d and %f).
• Strict-rules allow bool and null in string operations (inconsistent with %s).

Idea:

• Let's enable passing null to %s. If PHPStan later adds a config option to limit type coercions then it could be used here (as well as $string . $null, "{$null}", ...).
• Let's enable passing numeric-string to %f. I excluded it, because numeric-string could lose precision by doing so. But it's probably too strict.
• Let's keep passing bool to %s prohibited. I could later try to prohibit bool in $string . $bool and "{$bool}" in strict-rules (no promises).

Just a small side note, PHPStorm will tell me to remove (string) $null for %s in a sprintf. It's only fine with $null ?? ''.

Idea:

• Let's enable passing null to %s. If PHPStan later adds a config option to limit type coercions then it could be used here (as well as $string . $null, "{$null}", ...).

I don't know if that helps, but I've reviewed the new errors that popped up in my current project. The dominant error reported was that we pass something that could be null. And in most of the cases, it would be a problem is the value was actually null. It was also old news because we already knew that the code in question is a bit too lax when it comes to handling null. So all errors that the new rule added to our baseline were actually just "more of the same".

• Let's enable passing numeric-string to %f. I excluded it, because numeric-string could lose precision by doing so. But it's probably too strict.

Yes, it is. In fact, you actually might lose precision when casing a numeric string to a float. And besides it doesn't make much sense to cast a string to a float just to format it as a string again.